Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scripts checking #53

Merged
merged 10 commits into from
Jul 20, 2014
Merged

Scripts checking #53

merged 10 commits into from
Jul 20, 2014

Conversation

doktorbro
Copy link

WIP

Javascripts are another important source we could check. They behave technically mostly like images, except they can have content. Should I implement more?


</body>

</html>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I put scripts fixtures in fixtures/scripts?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right here is fine. In another PR I will move the files into directories for a cleanup. I'd rather not that happen in this PR as it will make the diff unfairly big. Thanks for asking! ❤️

@doktorbro
Copy link
Author

Looks complete to me.

end

def blank?
@content.empty?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would @content.strip.empty? be better?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I think so, for completeness sake.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@gjtorikian
Copy link
Owner

I think this makes sense. Two questions:

  1. Is it possible to catch self-closing script tags? Like, <script src="./blah.js" />. This is more a hypothetical question. Such a tag is considered not valid, but many times in my life I've made the mistake (rather than closing with </script>).
  2. Could I convince you to do another PR (or maybe within this PR?) for style tags?

@doktorbro
Copy link
Author

  1. I’ll try it. I hope Nokogiri can distinguish self-closing from empty content.

  2. What do you want to check in the style tag? How could it be invalid?

    <style>
    body { color: black; background: white; }
    em { font-style: normal; color: red; }
    </style>
    

@gjtorikian
Copy link
Owner

What do you want to check in the style tag?

Ugh, sorry, I promise I know how to computer. I meant

<link rel="stylesheet" type="text/css" href="styles.css">

Which is already covered.

@doktorbro
Copy link
Author

Nokogiri can not distinguish self-closing node from empty one. This element <script src="./blah.js" /> is already parsed to <script src="./blah.js"></script>. You don’t have the original state of element on this stage. That’s why I would not check for self-closing scripts.

@parkr
Copy link
Contributor

parkr commented Jul 16, 2014

You could use regexp for each script node to see if it's self-closing.

@doktorbro
Copy link
Author

@parkr I can’t figure out how because the node is already parsed:

node.to_s == '<script src="./blah.js"></script>'

You can add you solution on top of my PR, if you know how. Keep in mind this Stack Overflow answer.

@gjtorikian, you said in #30 (comment)

I don't think valid HTML is something I'm interested in here.

The self-closing check is HTML validation, so we could spare this check.

add_to_external_urls script.src
else
next if script.ignore?
self.add_issue("internal script #{script.src} does not exist") unless script.exists?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather this be rewritten as:

if script.missing_src?
  self.add_issue "script is empty and has no src attribute"
else
  next if script.ignore?
  if script.remote?
    add_to_external_urls script.src
  else
    self.add_issue("internal script #{script.src} does not exist") unless script.exists?

@doktorbro
Copy link
Author

Rebased.

gjtorikian added a commit that referenced this pull request Jul 20, 2014
@gjtorikian gjtorikian merged commit 2105ba0 into gjtorikian:master Jul 20, 2014
@doktorbro doktorbro deleted the feat-script branch July 20, 2014 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants